Skip to content

docs: prefer Protocol over ProtocolBase in cross-references, per the owner's #1103 ruling - #2420

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1103-protocol-not-protocolbase
Oct 28, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1103-protocol-not-protocolbase

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 28, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

Description of your pull request and other information

The owner ruled on #1103 the opposite of what both earlier rounds assumed: "ProtocolBase is the internal hidden API. Protocol is the one exposing the APIs and attributes. So I'd prefer we keep referring on Protocol whenever possible." Round 1 (#2408) already merged with the wrong-direction conversion in the register/register_protocol docs; round 2 (#2415) was closed unmerged for the same reason.

This PR reverts #2408's merged changes and finishes the direction correctly:

  • pcapkit/protocols/protocol.py, internet/internet.py, link/link.py, transport/transport.py, transport/sctp.py, foundation/registry/protocols.py — the register/register_protocol/register_protocol_code docs go back to Protocol.<member>, including fixing pre-existing display/target mismatches (ProtocolBase.register <...Protocol.register>-style) the same way.
  • One exception, kept deliberately: register's own Raises: clause still names ProtocolBase. docs(protocols): register's Raises name Protocol in some layers, ProtocolBase in others #2316/fix(foundation): registrar error messages and type hints name the public class, not the Base the guard checks #1016 already ruled on this split — the clause names the class the guard's issubclass check actually tests, while the raised message names Protocol. tests/protocols/test_register_class_guard_unit.py asserts this and caught a first pass of this PR that flipped it by mistake.
  • The 6 pre-existing sites already identified in protocol.py, misc/null.py, application/httpv2.py, the mptcp test and ipv6_ext.rst (one, schema/internet/ipv6_route.py, was already correct — no change needed there).
  • A targeted grep across pcapkit/ and docs/ for other ProtocolBase.<member> cross-reference targets where the member is plainly inherited (not something ProtocolBase uniquely has): __init__, __post_init__, _make_data, from_data, alias, length, packet, protocol, unpack, __keywords__, __own_make_keywords__, __data__, _lookup_next_layer, _import_next_layer, _read_packet, expand_comp — across sentinels.py, data/data.py, misc/pcapng.py, misc/pcap/{frame,header}.py, internet/{hopopt,esp,mh,ipv6,ah,ipv6_ext}.py, application/http.py, utilities/config.py and conf.py.
  • Left alone, deliberately: ProtocolBase.__init_subclass__ in link/l2tpv2.py, link/c_tag.py and protocol-layer-placement.rst — Protocol overrides __init_subclass__, so these name the base implementation's distinct schema/data-resolution behaviour on purpose, not the plain-inherited-member case. protocol-layer-placement.rst's __proto__ reference is a structural MRO discussion naming ProtocolBase explicitly as part of that argument, likewise left alone.

Round 3 refinement: the owner further ruled "use the last documented override" — within a class's own docstring describing what it overrides, the reference must point at the real ancestor in that class's MRO, not Protocol. Confirmed by probing the live hierarchy: Protocol.__subclasses__() is empty — every one of the 44 loaded layer classes (Internet, Link, Transport, HTTP, PCAPNG, etc.) descends from ProtocolBase only, never through Protocol. 21 override-framing sites ("the guard this shares with", "As X, but...", "Narrows... declared type", etc.) were reverted back to ProtocolBase across internet.py, link.py, transport.py, ipv6_ext.py(+rst), ah.py, esp.py, hopopt.py, mh.py, null.py, pcapng.py, http.py, httpv2.py; plain "here's where this is documented" links were left as Protocol. Three more sites (http.py's Protocol.__init__ ×2, ipv6_ext.py's Protocol.__own_make_keywords__) read the same way under a strict reading of the rule but weren't caught this round — flagging for a follow-up rather than blocking.

Scope not reached: ~37 test files under tests/protocols/ and tests/project/ carry the same ProtocolBase.<member> pattern (pre-existing, untouched by this PR either direction). Grep for the full list: grep -rln ':\(class\|meth\|attr\|func\):\[^`]protocol.ProtocolBase.[A-Za-z_]' --include=".py" tests/`. Left for a dedicated #1103 round, per scope.

Test plan

  • AST-structure guard — ast.dump via util/vendor_code_changed.py's code_of, with docstrings and bare string literals stripped: all touched .py files (18 across both rounds) are prose-only against 27c797d35, 0 differences.
  • tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py, tests/test_docstring_contract.py, tests/project/test_conventions_doc_claims.py, test_register_class_guard_unit.py — 100 passed, 22 skipped, 444 subtests passed.
  • Full tests/protocols, tests/foundation, tests/corekit, tests/utilities (make test's exclusions) — 3414 passed, 18 skipped, 60258 subtests passed, 0 failures. The first pass caught the Raises: clause mistake above (test_register_class_guard_unit.py, 6 SUBFAILED); fixed and reconfirmed green.

@JarryShaw JarryShaw added the docs Pull requests that change documentation only (docs: subject prefix) label Oct 28, 2026
@github-actions github-actions Bot added this to the PyPCAPKit v1.5 milestone Oct 28, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 28, 2026
@JarryShaw JarryShaw added the review: running A cross-review is in flight against the current head - no verdict yet label Oct 28, 2026
@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 91.73% (unit tier, Python 3.15, 891f85fa4, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19609 879 2620 866 92.13%
pcapkit/corekit 2596 74 818 37 96.51%
pcapkit/dumpkit 282 3 102 3 98.44%
pcapkit/foundation 3045 24 1062 26 98.78%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17592 193 4678 179 98.32%
pcapkit/toolkit 924 36 334 10 95.55%
pcapkit/utilities 502 7 144 5 98.14%
pcapkit/vendor 4449 1961 1020 167 52.64%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Review: GOOD TO GO at 6c972b83 (Sonnet). The tricky part held up under independent verification: the register Raises: exception is real and test-locked — live probe confirms the guard really checks issubclass(protocol, ProtocolBase), and a mutation test confirms test_raises_names_protocol_base_for_the_subclass_check catches a flip of it. Revert from #2408 confirmed complete (all 11 added lines gone). Spot-checked ~15 of the retargeted members — all 20 are documented under Protocol and present in the published objects.inv. The two __init_subclass__ sites kept are genuinely distinct code (confirmed by reading both implementations). AST guard: 23/23 prose-only. Test figures match exactly (59/1/217, 33/21/144, 3290/13/29785/0).

Non-blocking: the body understates the remaining backlog (54 non-test ProtocolBase.<member> roles remain, not the ~37 test-only figure quoted — 42 of those are genuinely convertible, including some siblings of sentences this PR already touched). Full list in the reviewer's scratch output — worth folding into the next #1103 round rather than this PR.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 28, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Use the last documented override. I see that we replaced the reference in the Internet to Protocol. That's not accepted.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 28, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Got it — confirmed Protocol is not actually in Internet's MRO (only ProtocolBase is), so a reference inside Internet's own docstring describing what it overrides has to point at the real ancestor, ProtocolBase, not the sibling Protocol. The earlier "prefer Protocol" ruling applies to plain "here's where this member is documented" links; override-describing links ("As X.method, but...", "the guard this shares with...") need the actual base in that class's own chain. Auditing the whole diff for this distinction and fixing it.

)

ProtocolBase is the internal hidden API and Protocol is the one exposing the
attributes, so a plain "here is where this member is documented" link targets
Protocol. The owner then refined this: "Use the last documented override." A
docstring describing what a *specific* class overrides has to name the real
ancestor in that class's own chain, which is never Protocol -- Protocol has no
subclasses at all, so ProtocolBase is the last documented override for every
class in the tree.

* Plain documentation links go to Protocol.<member>: the module-level prose and
  `#:` comments in protocol.py, register_protocol/_register_name in
  registry/protocols.py, the `register` Arguments and versionchanged clauses in
  internet.py, link.py, transport.py and sctp.py, and the unrelated-class
  references in sentinels.py, data/data.py, misc/pcap/{frame,header}.py,
  ipv6.py, utilities/config.py, conf.py and the mptcp test.
* Override-describing links go to ProtocolBase.<member>: Internet.{register,
  from_data,_make_payload}, Link.register, Transport.register, IPv6_Ext.protocol,
  the ipv6_ext module docstring and ipv6_ext.rst, the {AH,ESP,HOPOPT,MH}.alias
  MRO sentences, NoPayload.protochain, PCAPNG.{length,packet}, HTTP.from_data and
  HTTPv2's HTTP.unpack. Each of these says "As X", "the base", "the inherited",
  "narrows", "sits between ... in the MRO" or "the guard this shares with", and
  the surrounding sentences already name ProtocolBase.
* The `Raises:` clause of each `register` keeps naming ProtocolBase, unchanged:
  #2316/#1016 ruled that it names the class the guard's `issubclass` actually
  tests while the raised message names Protocol, and
  tests/protocols/test_register_class_guard_unit.py asserts exactly that.
* Class-attribute declaration pointers stay on Protocol -- Protocol.__data__
  (declared on Protocol itself), Protocol.__keywords__ and
  Protocol.__own_make_keywords__ -- since they name where the knob is documented
  rather than a method a class overrides.
* ProtocolBase.__init_subclass__ references are still left alone (l2tpv2.py,
  c_tag.py, protocol-layer-placement.rst): Protocol does override
  __init_subclass__, so those name the base implementation deliberately.

Verified Protocol in no class's __mro__ but its own, and that ProtocolBase
defines and documents every member involved while Protocol declares only
__init_subclass__ and __data__.

AST-structure guard (ast.dump with docstrings stripped) confirms all 12 touched
.py files are prose-only against 27c797d; ipv6_ext.rst carries no code. The
mptcp test, test_docstring_contract.py, test_conventions_doc_claims.py,
test_register_class_guard_unit.py and the docs xref suites pass (91 passed, 22
skipped, 444 subtests), as do tests/protocols, tests/foundation, tests/corekit
and tests/utilities in full (3414 passed, 18 skipped, 60258 subtests).
@JarryShaw
JarryShaw force-pushed the fix/1103-protocol-not-protocolbase branch from 6c972b8 to 891f85f Compare October 28, 2026 21:03
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 28, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Review: GOOD TO GO at 891f85fa (Sonnet, round 3). Independently verified the MRO claim across 10 different classes — Protocol genuinely has zero subclasses. Read all 21 reverted sites in context, confirmed genuine override-framing language in each. All 5 net-unchanged files confirmed byte-identical to the pre-#2408 baseline. AST guard clean (0 non-prose diffs across all 18 files from both rounds). Tests match exactly: 3414 passed/18 skipped/60258 subtests.

Three sites flagged as non-blocking (http.py's Protocol.__init__ ×2, ipv6_ext.py's Protocol.__own_make_keywords__) read as override-framing under a strict reading but weren't caught this round — noted in the body for a follow-up rather than blocking this PR. Fixed the stale body (file/test counts, added the round-3 summary) directly.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 28, 2026
@JarryShaw
JarryShaw merged commit 867616b into main Oct 28, 2026
65 checks passed
@JarryShaw
JarryShaw deleted the fix/1103-protocol-not-protocolbase branch October 28, 2026 22:58
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 28, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Updating the 21 reverted sites to the split display/target form: :meth:Protocol.X <...ProtocolBase.X>`` — shows Protocol.X, links to `ProtocolBase.X` where the real doc entry is. Pulling this PR back.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: good-to-go Cross-review at the current head says ready; CI state is separate review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 28, 2026
JarryShaw added a commit that referenced this pull request Oct 28, 2026
The owner refined the #1103 ruling: a `ProtocolBase.<member>` reference should
read `Protocol.<member>` in the rendered docs, for one consistent public
vocabulary, while the link still targets the `ProtocolBase` attribute. That is
the split-display-target form, `:meth:`Protocol.X <...protocol.ProtocolBase.X>``,
applied across everything Sphinx renders.

* 46 reference sites in `pcapkit/` and `docs/source/` now display
  `Protocol.<member>`. 37 keep their `ProtocolBase` target, which is the
  requested form; the other 9 already targeted `Protocol` -- #2420 moved those
  targets but left the display text reading `ProtocolBase` -- and are now
  display-consistent too.
* Three short-form references in `protocol.py` gained an explicit
  fully-qualified target, so the display text could change without repointing
  the link: `:meth:`ProtocolBase._parse_next_layer`` is now
  `:meth:`Protocol._parse_next_layer <...ProtocolBase._parse_next_layer>``.
* `__init_subclass__` keeps displaying `ProtocolBase` (6 sites). `Protocol`
  defines its own override, so displaying `Protocol` there would name a
  different method than the link resolves to.
* Two further sites keep `ProtocolBase` because the adjacent prose would
  otherwise contradict itself: `schema.py`'s comment reads "``ProtocolBase``,
  not ``Protocol``" two lines above its reference, and
  `protocol-layer-placement.rst` asserts `OSPF.__proto__` *is* that attribute
  inside a paragraph analysing MRO set-difference.
* The `Raises:` clause of `register` is unchanged. It names the class the
  guard's `issubclass` actually tests, not a member whose documentation is
  being pointed at, and `tests/protocols/test_register_class_guard_unit.py`
  asserts that wording (#2316/#1016).
* `tests/` and `examples/` are untouched (28 sites). Their docstrings are not
  part of the Sphinx render, so the display rationale does not reach them.

No link was repointed: a guard canonicalises every role target before and
after and reports them identical, bar the three short-form expansions, which
resolve to the same object. An AST-structure guard (`ast.dump` with docstrings
stripped) confirms all 26 touched `.py` files are prose-only.
JarryShaw added a commit that referenced this pull request Oct 29, 2026
Applying the #1103 ruling to the member references #2420 left behind: a
`ProtocolBase.<member>` cross-reference now reads `Protocol.<member>`, for one
consistent public vocabulary, and links there too.

Both halves move, because on the rendered page that is where the documentation
is. `protocol.rst` documents the members with nested
`autoproperty`/`automethod` directives under `.. autoclass:: ...Protocol`, so
`Protocol.<member>` carries the signature and the docstring; the `ProtocolBase`
block is `:no-members:` plus grouped `:no-index-entry:` stubs whose body only
links back to the `Protocol.*` entries, and the general index lists `Protocol.*`
alone. Measured on the pinned docs toolchain (`Sphinx==9.2.0`).

* 46 sites in `pcapkit/` and `docs/source/`. 37 move display *and* target to
  `Protocol.<member>`; the other 9 already targeted `Protocol` and only needed
  the display text. The explicit target is the fully-qualified spelling the
  house style asks for -- the form 124 existing references already use.
* `from_data` is consistent again: `internet.py` and `application.py` pointed
  at `ProtocolBase.from_data` where `link.py` and `transport.py` pointed at
  `Protocol.from_data` for the same kind of reference. All four now read alike.
* `__init_subclass__` keeps `ProtocolBase` (6 sites), since `Protocol`
  overrides it and naming `Protocol` would name a different method. So do
  `schema.py` and `protocol-layer-placement.rst`, whose adjacent prose would
  otherwise contradict itself, and `register`'s `Raises:` clause, which names
  the class the guard's `issubclass` tests (#2316/#1016).
* `changelog/1.5.0.rst` keeps its five `ProtocolBase.<member>` references.
  Each names the class a *built-in* protocol inherits the member from -- the
  built-ins derive from `ProtocolBase`, not `Protocol` -- and `CHANGELOG.md` is
  generated from that file by `util/changelog_md.py`, where a role flattens to
  a code span with no link, so moving them would rewrite release-note prose
  rather than an href.
* `tests/` and `examples/` are untouched (28 sites): their docstrings are not
  part of the Sphinx render.

Hrefs only. A role-canonicalisation guard reports no role and no display-text
change beyond the above, and an AST-structure guard (`ast.dump` with docstrings
stripped) is identical for every touched `.py` file.
JarryShaw added a commit that referenced this pull request Oct 29, 2026
 display rule (#2422)

Applying the #1103 ruling to the member references #2420 left behind: a
`ProtocolBase.<member>` cross-reference now reads `Protocol.<member>`, for one
consistent public vocabulary, and links there too.

Both halves move, because on the rendered page that is where the documentation
is. `protocol.rst` documents the members with nested
`autoproperty`/`automethod` directives under `.. autoclass:: ...Protocol`, so
`Protocol.<member>` carries the signature and the docstring; the `ProtocolBase`
block is `:no-members:` plus grouped `:no-index-entry:` stubs whose body only
links back to the `Protocol.*` entries, and the general index lists `Protocol.*`
alone. Measured on the pinned docs toolchain (`Sphinx==9.2.0`).

* 46 sites in `pcapkit/` and `docs/source/`. 37 move display *and* target to
  `Protocol.<member>`; the other 9 already targeted `Protocol` and only needed
  the display text. The explicit target is the fully-qualified spelling the
  house style asks for -- the form 124 existing references already use.
* `from_data` is consistent again: `internet.py` and `application.py` pointed
  at `ProtocolBase.from_data` where `link.py` and `transport.py` pointed at
  `Protocol.from_data` for the same kind of reference. All four now read alike.
* `__init_subclass__` keeps `ProtocolBase` (6 sites), since `Protocol`
  overrides it and naming `Protocol` would name a different method. So do
  `schema.py` and `protocol-layer-placement.rst`, whose adjacent prose would
  otherwise contradict itself, and `register`'s `Raises:` clause, which names
  the class the guard's `issubclass` tests (#2316/#1016).
* `changelog/1.5.0.rst` keeps its five `ProtocolBase.<member>` references.
  Each names the class a *built-in* protocol inherits the member from -- the
  built-ins derive from `ProtocolBase`, not `Protocol` -- and `CHANGELOG.md` is
  generated from that file by `util/changelog_md.py`, where a role flattens to
  a code span with no link, so moving them would rewrite release-note prose
  rather than an href.
* `tests/` and `examples/` are untouched (28 sites): their docstrings are not
  part of the Sphinx render.

Hrefs only. A role-canonicalisation guard reports no role and no display-text
change beyond the above, and an AST-structure guard (`ast.dump` with docstrings
stripped) is identical for every touched `.py` file.
JarryShaw added a commit that referenced this pull request Oct 29, 2026
…ce sweep, fix other prose drift (#2428)

Round 8 of the whole-repo prose sweep (#1103). Fanned out 8 shard agents by
directory; most of the repo was clean. Changes:

* pcapkit/protocols/internet/ipv6_ext.py + its .rst: finish the carry-over
  miss from round 7 -- the `_lookup_next_layer` plain pointer still targeted
  `ProtocolBase`, flipped to `Protocol` per #2420's ruling.
* Same miss in corekit/sentinels.py (`__keywords__`), docs/source/ext.rst
  (three sites, including a verbatim twin of a paragraph #2420 already fixed
  in protocol.py), schema/schema.py and schema/link/ethernet.py.
* internet.py, link.py, transport.py, sctp.py, protocol.py: `register()`'s
  Arguments: and a couple of Returns:/versionchanged clauses said `Protocol`
  subclass/instance while the signature types `ProtocolBase` and the
  adjacent Raises: clause already said so -- internal contradiction, fixed
  to `ProtocolBase` to match.
* docs/source/contributing/conventions/protocol-layer-placement.rst: the
  Link-Application-ProtocolBase set difference is `__proto_key__`, not
  empty as claimed (#2343 added it after this page was written).
* docs/source/contributing/conventions/registry-protocol.rst: "every
  registry overrides `_validate_value`" contradicted its own next
  paragraph, where the AppType transports inherit rather than override.
* pcapkit/vendor/ipx/packet.py, vendor/reg/linktype.py: a self-contradicting
  claim and a stale counted-number, both in vendor docstrings.
* docs/source/engines.rst, docs/source/conf.py, util/changelog_md.py,
  CONTRIBUTING.md: a stale scapy version, a wrong file path, a false
  "matches the Sphinx we build with" claim, and a missing caveat about
  actionlint making Lint non-advisory.

No executable code changed -- every edit is inside a docstring, comment, or
.rst file; confirmed by diff review and `import pcapkit` from this worktree.

Four behavioral defects found but deliberately left unfixed (reported
separately): 16 of 24 pcapkit/vendor crawlers' `process(self, data)` reads
the module-global `DATA` instead of its own `data` argument (latent,
harmless only because `request()` returns that same global); an empty
`FilterType` registry behind a live TODO; three open "handle this
programmatically" TODOs in pcapkit/vendor/mh; and dead `multiprocessing.Pool`
scaffolding in vendor/__main__.py.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Pull requests that change documentation only (docs: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant